Skip to content

Conversation

@soheilshahrouz
Copy link
Contributor

@soheilshahrouz soheilshahrouz commented Oct 28, 2025

Removes the pin_offset attribute from the architecture file, which was previously used to model 3D OPIN connectivity.
Instead, the <fc_override> tag is now used to specify absolute fc values for connecting OPINs to CHANZ wire segments.

Additionally, several functions have been moved from rr_graph2.cpp to two new files: rr_graph_chan_chan_edges.cpp and rr_graph_opin_chan_edges.cpp.

@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool libarchfpga Library for handling FPGA Architecture descriptions lang-cpp C/C++ code labels Oct 28, 2025
@soheilshahrouz soheilshahrouz changed the title [WIP] 3D OPIN-CHANZ connectivity 3D OPIN-CHANZ connectivity Nov 10, 2025
Copy link
Contributor

@AmirhosseinPoolad AmirhosseinPoolad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a PR, Soheil. Had some thoughts, mostly on styling. I did not review rr_graph_chan_chan_edges.cpp/h and rr_graph_opin_chan_edges.cpp/h as you said it was only moving the code to a new file.

Also only reviewed add_edges_opin_chanz_per_side and add_edges_opin_chanz_per_block in rr_graph_sg for the same reason.

e_parallel_axis parallel_axis,
bool keep_original_index = false);

int get_parallel_seg_index(int abs,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs doxygen

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

e_rr_type rr_type) const;


std::vector<RRNodeId> find_pin_nodes_at_side(int layer,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doxygen

scatter_wire_candidates.size());

continue;
// continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented out code

Comment on lines +404 to +411
seg_idx = get_parallel_seg_index(drive_seg_idx, indices_map, e_parallel_axis::X_AXIS);
drive_seg_idx = (seg_idx >= 0) ? seg_idx : drive_seg_idx;

get_parallel_seg_index(left_seg_idx, e_parallel_axis::X_AXIS, indices_map);
seg_idx = get_parallel_seg_index(left_seg_idx, indices_map, e_parallel_axis::X_AXIS);
left_seg_idx = (seg_idx >= 0) ? seg_idx : left_seg_idx;

get_parallel_seg_index(right_seg_idx, e_parallel_axis::X_AXIS, indices_map);
seg_idx = get_parallel_seg_index(right_seg_idx, indices_map, e_parallel_axis::X_AXIS);
right_seg_idx = (seg_idx >= 0) ? seg_idx : right_seg_idx;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename drive_seg_idx, left_seg_idx and right_seg_idx to have underlines after them. Not clear in a glance that they're private fields and not consistent with the rest of the code base.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain the logic of this code. Not sure why we are doing the set either the reutnred value or the old value as the new value and that looks like new behaviour. It should be explained.

* @param perturb_switch_pattern Specifies whether connections should be distributed unevenly across the channel or not.
* @param directionality Segment directionality: should be either *UNI-DIRECTIONAL* or *BI-DIRECTIONAL*
* @param seg_inf Segments type information, such as length, frequency, and etc.
* @param sets_per_seg_type Number of available sets within the channel_width of each segment type.
*
* @return an 5D matrix which keeps the track indicies connected to each pin ([0..num_pins-1][0..width-1][0..height-1][0..layer-1][0..sides-1]).
* @return an 4D matrix which keeps the track indices connected to each pin ([0..num_pins-1][0..width-1][0..height-1][0..layer-1][0..sides-1]).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still 5 dimensions in the comment. '([0..num_pins-1][0..width-1][0..height-1][0..layer-1][0..sides-1])'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

layer should be removed from the comment I think.

t_rr_edge_info_set& rr_edges_to_create,
const vtr::NdMatrix<std::vector<t_bottleneck_link>, 2>& interdie_3d_links);

void add_edges_opin_chanz_per_block(const RRGraphView& rr_graph,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doxygen

Comment on lines 16 to +17
* [0..device_ctx.physical_tile_types.size()-1][0..num_pins-1][0..width][0..height][0..layer-1][0..3][0..Fc-1] */
typedef std::vector<vtr::NdMatrix<std::vector<int>, 5>> t_pin_to_track_lookup;
typedef std::vector<vtr::NdMatrix<std::vector<int>, 4>> t_pin_to_track_lookup;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comment above (remove layer)

*
* Note that when we model different channels based on position not axis, we can't use this anymore and need to have a lookup for each grid location. */
typedef std::vector<vtr::NdMatrix<std::vector<int>, 5>> t_track_to_pin_lookup;
typedef std::vector<vtr::NdMatrix<std::vector<int>, 4>> t_track_to_pin_lookup;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comment above.

Comment on lines +227 to +240
<!-- Gather 30 connections from the end of L4 wires at all four sides of a switchblock location -->
<wireconn num_conns="0" from_type="L1" from_switchpoint="1" side="rltb"/>
</gather>
<scatter>
<!-- Scatter 30 connections to the start of L4 wires at all four sides of a switchblock location -->
<wireconn num_conns="30" to_type="L1" to_switchpoint="0" side="rtlb"/>
</scatter>
<sg_link_list>
<!-- Link going up one layer, using the 'seg4_inter_die_driver' multiplexer to gather connections from the bottom layer and using the LZ node/wire to move up one layer -->
<sg_link name="L_UP" z_offset="1" x_offset="0" y_offset="0" mux="segz_driver" seg_type="LZ"/>
<!-- Link going down one layer, using the 'seg4_inter_die_driver' multiplexer to gather connections from the top layer and using the LZ node/wire to down up one layer -->
<sg_link name="L_DOWN" z_offset="-1" mux="segz_driver" seg_type="LZ"/>
</sg_link_list>
<!-- Instantiate 6 'L_UP' and 6 'L_DOWN' sg_links per switchblock location everywhere on the device -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comments here to represent the architecture.

Comment on lines +48343 to +48364
<!-- Specify a scatter-gather pattern for 3D connections. -->
<scatter_gather_list>
<sg_pattern name="name" type="unidir">
<gather>
<!-- Gather 30 connections from the end of L4 wires at all four sides of a switchblock location -->
<wireconn num_conns="0" from_type="L4" from_switchpoint="3" side="rltb"/>
</gather>
<scatter>
<!-- Scatter 30 connections to the start of L4 wires at all four sides of a switchblock location -->
<wireconn num_conns="30" to_type="L4" to_switchpoint="0" side="rtl"/>
</scatter>
<sg_link_list>
<!-- Link going up one layer, using the 'seg4_inter_die_driver' multiplexer to gather connections from the bottom layer and using the LZ node/wire to move up one layer -->
<sg_link name="L_UP" z_offset="1" x_offset="0" y_offset="0" mux="seg4_inter_die_driver" seg_type="LZ"/>
<!-- Link going down one layer, using the 'seg4_inter_die_driver' multiplexer to gather connections from the top layer and using the LZ node/wire to down up one layer -->
<sg_link name="L_DOWN" z_offset="-1" mux="seg4_inter_die_driver" seg_type="LZ"/>
</sg_link_list>
<!-- Instantiate 6 'L_UP' and 6 'L_DOWN' sg_links per switchblock location everywhere on the device -->
<sg_location type="EVERYWHERE" num="60" sg_link_name="L_UP"/>
<sg_location type="EVERYWHERE" num="60" sg_link_name="L_DOWN"/>
</sg_pattern>
</scatter_gather_list>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure there is a comment at the top of the arch file explaining the 3D part.

Copy link
Contributor

@vaughnbetz vaughnbetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup and overall well structured code. Needs more commenting though in places.

e_parallel_axis parallel_axis,
bool keep_original_index = false);

int get_parallel_seg_index(int abs,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

}

/**
* @brief This loads up VPR's arch_switch_inf data by combining the switches
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move this doxygen comment to the declaration of the function near the top of the file.

int curr_layer_num = rr_graph.node_layer_low(curr.node);
if (curr_rr_type == e_rr_type::CHANX || curr_rr_type == e_rr_type::CHANY || curr_rr_type == e_rr_type::SINK) {
//We stop expansion at any CHANX/CHANY/SINK
if (curr_rr_type == e_rr_type::CHANX || curr_rr_type == e_rr_type::CHANY || curr_rr_type == e_rr_type::CHANZ || curr_rr_type == e_rr_type::SINK) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to add CHANZ in some of the ASCII art above this line.

}

if (curr_rr_type == e_rr_type::CHANZ) {
curr_layer_num = (root_layer_num + 1) % 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain why ... I'm not sure why this is done at all.

Comment on lines +404 to +411
seg_idx = get_parallel_seg_index(drive_seg_idx, indices_map, e_parallel_axis::X_AXIS);
drive_seg_idx = (seg_idx >= 0) ? seg_idx : drive_seg_idx;

get_parallel_seg_index(left_seg_idx, e_parallel_axis::X_AXIS, indices_map);
seg_idx = get_parallel_seg_index(left_seg_idx, indices_map, e_parallel_axis::X_AXIS);
left_seg_idx = (seg_idx >= 0) ? seg_idx : left_seg_idx;

get_parallel_seg_index(right_seg_idx, e_parallel_axis::X_AXIS, indices_map);
seg_idx = get_parallel_seg_index(right_seg_idx, indices_map, e_parallel_axis::X_AXIS);
right_seg_idx = (seg_idx >= 0) ? seg_idx : right_seg_idx;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain the logic of this code. Not sure why we are doing the set either the reutnred value or the old value as the new value and that looks like new behaviour. It should be explained.

const int grid_height = grid.height();

sb_loc0.x = std::clamp(sb_loc0.x, 0, grid_width - 1);
sb_loc0.y = std::clamp(sb_loc0.y, 0, grid_height - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may be able to shorten this routine by putting the sb_loc0 etc. into a vector with two elements and looping. Or you could use helper functions as @AmirhosseinPoolad suggests.

<sg_pattern name="name" type="unidir">
<gather>
<!-- Gather 30 connections from the end of L4 wires at all four sides of a switchblock location -->
<wireconn num_conns="0" from_type="L1" from_switchpoint="1" side="rltb"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should num_conns be 30 instead of 0 here?

<loc side="top" layer_offset="1">io.outpad io.inpad io.clock</loc>
<loc side="right" layer_offset="1">io.outpad io.inpad io.clock</loc>
<loc side="bottom" layer_offset="1">io.outpad io.inpad io.clock</loc>
<loc side="left">io.outpad io.inpad io.clock</loc>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment at the top of this arch file explaining what it is.

<sg_pattern name="name" type="unidir">
<gather>
<!-- Gather 30 connections from the end of L4 wires at all four sides of a switchblock location -->
<wireconn num_conns="0" from_type="L4" from_switchpoint="3" side="rltb"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be num_conns = 30?

Comment on lines +48343 to +48364
<!-- Specify a scatter-gather pattern for 3D connections. -->
<scatter_gather_list>
<sg_pattern name="name" type="unidir">
<gather>
<!-- Gather 30 connections from the end of L4 wires at all four sides of a switchblock location -->
<wireconn num_conns="0" from_type="L4" from_switchpoint="3" side="rltb"/>
</gather>
<scatter>
<!-- Scatter 30 connections to the start of L4 wires at all four sides of a switchblock location -->
<wireconn num_conns="30" to_type="L4" to_switchpoint="0" side="rtl"/>
</scatter>
<sg_link_list>
<!-- Link going up one layer, using the 'seg4_inter_die_driver' multiplexer to gather connections from the bottom layer and using the LZ node/wire to move up one layer -->
<sg_link name="L_UP" z_offset="1" x_offset="0" y_offset="0" mux="seg4_inter_die_driver" seg_type="LZ"/>
<!-- Link going down one layer, using the 'seg4_inter_die_driver' multiplexer to gather connections from the top layer and using the LZ node/wire to down up one layer -->
<sg_link name="L_DOWN" z_offset="-1" mux="seg4_inter_die_driver" seg_type="LZ"/>
</sg_link_list>
<!-- Instantiate 6 'L_UP' and 6 'L_DOWN' sg_links per switchblock location everywhere on the device -->
<sg_location type="EVERYWHERE" num="60" sg_link_name="L_UP"/>
<sg_location type="EVERYWHERE" num="60" sg_link_name="L_DOWN"/>
</sg_pattern>
</scatter_gather_list>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure there is a comment at the top of the arch file explaining the 3D part.

Copy link
Contributor

@amin1377 amin1377 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, Soheil! Thanks! I don’t have much to add to what Amir and Vaughn already mentioned. Just one question: have you explained fc_override and how it helps with OPIN connections to other layers in the document? Thanks!

limited_to_opin = false;
break;
}
e_rr_type to_type = rr_graph.node_type(to_node);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s not directly related to this section, but it occurred to me while reading this part. I think we assume that CHANZ is definitely stretched across multiple layers. If that’s correct, do we have a check for this in the RR graph validation to ensure that assumption holds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lang-cpp C/C++ code libarchfpga Library for handling FPGA Architecture descriptions libvtrutil VPR VPR FPGA Placement & Routing Tool

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants